Skip to content

Add allowed UUIDs and ignore CEC to Google Cast options flow#47269

Merged
MartinHjelmare merged 10 commits into
home-assistant:devfrom
emontnemery:cast_options
Mar 25, 2021
Merged

Add allowed UUIDs and ignore CEC to Google Cast options flow#47269
MartinHjelmare merged 10 commits into
home-assistant:devfrom
emontnemery:cast_options

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

@emontnemery emontnemery commented Mar 2, 2021

Breaking change

  • The configuration.yaml for the cast integration has been deprecated. It will become invalid in core-2021.6.0.

Proposed change

Make allowed UUIDs and ignore CEC lists configurable through Google Cast options flow
Deprecate cast yaml config

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 16, 2021

Rebase?

from .const import DOMAIN

# Deprecated from 2021.4, remove in 2021.6
CONFIG_SCHEMA = cv.deprecated(DOMAIN)
Copy link
Copy Markdown
Member

@balloob balloob Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, we had no config schema to begin with and just passed that to import? 🤔

Comment on lines +155 to +159
_add_with_suggestion(fields, CONF_KNOWN_HOSTS, suggested_value)
suggested_value = _list_to_string(current_config.get(CONF_UUID))
_add_with_suggestion(fields, CONF_UUID, suggested_value)
suggested_value = _list_to_string(current_config.get(CONF_IGNORE_CEC))
_add_with_suggestion(fields, CONF_IGNORE_CEC, suggested_value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know if this flow is requested by an advanced user. Let's hide UUID and IGNORE CEC unless users are advanced.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4487596

Comment thread homeassistant/components/cast/config_flow.py Outdated
Comment thread homeassistant/components/cast/config_flow.py Outdated
if CONF_UUID in config:
wanted_uuid = config[CONF_UUID]
# Import CEC IGNORE attributes
pychromecast.IGNORE_CEC += config_entry.data.get(CONF_IGNORE_CEC) or []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird that we're modifying a constant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's how the lib is designed. It could be refactored to a more sane API, with a setter function, but not in this PR.

cfg = ENTITY_SCHEMA(cfg)
media_player_config_validated.append(cfg)
except vol.Error as ex:
_LOGGER.warning("Invalid config '%s': %s", cfg, ex)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we accept a partial valid config to be more backwards compatible? The alternative would be to extend the config schema instead which would fail the whole config on invalid result.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the intention is to accept a partial valid config by simply skipping invalid media players instead of rejecting the entire config.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally the config flow tests should be part of a module test_config_flow.py.

Comment thread tests/components/cast/test_init.py
@MartinHjelmare MartinHjelmare merged commit 3188f79 into home-assistant:dev Mar 25, 2021
@MartinHjelmare
Copy link
Copy Markdown
Member

Marking as breaking change to highlight the YAML deprecation.

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants